-
-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use direnv-builtin DIRENV_WATCHES
to list watched files
#414
Conversation
EDIT: Wait...I reacted too quickly. This doesn't wrap |
Somewhat contradictory... You respect them and their software, too much, but don't care for its implementation? In any case, it is what it is, and I doubt they're gonna change it. For reference, the implementation is at https://github.com/direnv/direnv/blob/master/gzenv/gzenv.go. Objectively, there's nothing very difficult about The implementation here is a simple bash pipeline. It cannot be simpler. An equivalent Python one-liner is below, if you prefer, but this of course requires a Python interpreter, which I thought a bit heavy. python -c "import base64, json, os, zlib; print('\n'.join([p['Path'] for p in json.loads(zlib.decompress(base64.urlsafe_b64decode(os.environ['DIRENV_WATCHES'])).decode()) if p['Exists'] and not p['Path'].startswith(os.environ['XDG_DATA_HOME'] + '/direnv/allow')]))" (I should have commented in the patch. The In any case, as covered in #408, You advertise as:
This is (currently) incorrect. I appreciate your software and want to use it, but cannot as it stands. Do you agree that there is a problem? |
I very often respect the net result of a developer's code, but disagree on implementation details.
I've read it, thanks though.
"Difficult" != "Complex"
No - it does not add a hard dependency. #408 details how to work around the exposed interface inconsistencies. Alias
As I made clear in my responses on #408 - I personally think that trying to match the names and interface exposed by the standard direnv implementation of Please do not take my initial reaction as indicative of some overall apathy in fixing this or some dislike of the solution - you did what you had to do to fix the problem in this software. My frustration stemmed from the fact that it appeared initially that you'd ignored my statements in #408 and opened a PR fixing the problem in the exact way I had said I didn't want to do it. This seems to do the right thing in light of the underlying implementation of |
Thank you for your considered reply. Just to touch on the proposed workaround. It doesn't work for my (and others, @infinisil) use cases. "Hard dependency" was perhaps the wrong term, but I can't justify extra noise in I believe that matching the direnv interface is a good thing, and that it would be to this project's advantage if it was a pure drop-in over direnv, requiring no boilerplate. |
I'd still like to see what @Mic92 has to say on the matter, but I think I'm okay with merging this. It does just enough to integrate with the direnv internals to alleviate the need for a wrapping function but not more than necessary. I haven't given thought to the exception you made for I have opened a discussion about what it would take to merge our version of |
I've pushed an updated version with a polyfill using I also changed the parsed watches to be NUL-separated which should be more robust. |
Is it really better to do this instead of just overriding |
We don't want to override more builtins. You can also see the complexity that comes from overriding just two builtins. |
@kingarrrt I also added the functionality to direnv itself: direnv/direnv#1198 Only caveat is debian and other slow moving targets. |
bfc5da9
to
6366a95
Compare
|
||
_nix_direnv_watches() { | ||
local -n _watches=$1 | ||
while IFS= read -r line; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can drop this code when we get direnv/direnv#1198
Ok. After my refactoring this touches a lot less builtins in direnv. And if we get the PR merged it will be even less. Also now it no longer adds any new dependencies. |
I am aware of these plans, but to me direnv's stdlib is also one of the biggest strengths. If someone has to check 5 different places to get high-quality plugins for each ecosystem (the nodejs community would probably end up with 3 for the same thing) than this is worse user experience. I trust zimbatm to be more thoughtful when it comes to backwards compatibility compared to many other project maintainers. |
IMHO if the nodejs community shoots themselves in the foot with this change, I'm not sure I think it is direnv's problem in the first place. There are things I think we would need to implement (naming conventions, a plugin repository with a basic search feature or some other way to find plugins that've been "blessed") and we'd need to find maintainers for the separate implementations (or maybe only for the "sufficiently complex" ones), but I think that the separation of implementation out into smaller chunks might help improve the contributor base to each specialized bit. It's certainly more managerial work to make everything work together, but it makes the technical bits more approachable imo. |
nix_direnv_watch_file
is replaced with a function that wrapswatch_file
and issues a deprecation warning.See #408 (which this fixes) for discussion.